Implement `searchable` property for marker schema by canova · Pull Request #4352 · firefox-devtools/profiler

canova · 2022-12-02T14:18:56Z

We also need to land the Bugzilla counterpart here. It Adds searchable: true to:

Markers that we had manual checks for in the frontend, like: Log, FileIO, DOMEvent,
Markers that had generic payload fields like name and category.

This PR removes the manual filtering that we had and replaces it with checking all the payload's searchable fields.
We also had to add an upgrader for both gecko and processed format and add searchable fields for these fields, so even though we remove these manual checks, search on older profiles work as expected.

I wasn't sure if the performance would be the same but after doing some micro benchmarks, I've found out that it's at lest the same if not better. Here's a before and after running times of that getSearchFilteredMarkerIndexes that I measured.

Deploy preview:
I also got this profile from out spreadsheet that says "Awful lots of markers" :)
Before / After

codecov · 2022-12-02T14:25:48Z

Codecov Report

Base: 88.37% // Head: 88.36% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (35c1c5c) compared to base (02cd177).
Patch coverage: 89.33% of modified lines in pull request are covered.

❗ Current head 35c1c5c differs from pull request most recent head 8de1e7b. Consider uploading reports for the commit 8de1e7b to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4352      +/-   ##
==========================================
- Coverage   88.37%   88.36%   -0.01%     
==========================================
  Files         282      282              
  Lines       25314    25351      +37     
  Branches     6826     6828       +2     
==========================================
+ Hits        22371    22402      +31     
- Misses       2731     2737       +6     
  Partials      212      212

Impacted Files	Coverage Δ
src/selectors/per-thread/markers.js	`93.63% <ø> (ø)`
src/test/fixtures/profiles/marker-schema.js	`100.00% <ø> (ø)`
src/profile-logic/marker-schema.js	`89.18% <84.61%> (-0.29%)`	⬇️
src/profile-logic/processed-profile-versioning.js	`85.34% <86.95%> (+0.04%)`	⬆️
src/profile-logic/gecko-profile-versioning.js	`89.12% <88.88%> (-0.02%)`	⬇️
src/app-logic/constants.js	`100.00% <100.00%> (ø)`
src/profile-logic/marker-data.js	`93.64% <100.00%> (+0.06%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

julienw

This looks pretty good to me!

I don't see an upgrader part for these markers: https://searchfox.org/mozilla-central/rev/404408660a4d976e2ac25881cb1e1f2712f2d430/tools/profiler/core/MicroGeckoProfiler.cpp#47-88
do you think we could add one?

julienw · 2022-12-05T13:44:35Z

+        const searchableFields = schema.data.filter((field) =>
+          searchableFieldKeys.includes(field.key)
+        );
+        for (const field of searchableFields) {


optional nit:

Doing this in 2 steps look a bit more complicated than how it could be. What about looping on schema.data directly?

for (const field of schema.data) { if (searchableFieldKeys.includes(field.key)) { field.searchable = true; } }

Same comment for the other versioning file

Right, changed it, thanks :)

julienw · 2022-12-05T13:53:01Z

+            // 'target' wasn't included in our code before. But I thought this
+            // would be a useful addition.


good idea! maybe target wasn't existing at the time we implemented the custom search. Not sure :-)

Yeah, it looks like this was added after the initial implementation.

julienw · 2022-12-05T14:01:44Z

+          case 'FileIO': {
+            searchableFieldKeys = [
+              'filename',
+              'operation',
+              'source',
+              'threadId',
+            ];


Is this change necessary? I see it was already done in the gecko code.

It's probably not. I wasn't sure if they were added initially when we implemented the markers 2.0 or added later but it looks like they are actually added while we are implementing schemas. So we don't need to change them except threadId. We don't have the threadId in the schema, so added it manually.

julienw · 2022-12-05T15:18:29Z

+  markerSchema: MarkerSchema,
+  marker: Marker,
+  searchRegExp: RegExp
+): boolean {


I wonder if we should set searchRegExp.lastIndex = 0 here too. Currently we should be good, but I'm concerned about how we might use this function in the future too.

Yeah, that makes sense. Added it.

canova

Thanks for the review! I addressed your comments. Added the last commit, would you mind taking a quick look on it?

I don't see an upgrader part for these markers: https://searchfox.org/mozilla-central/rev/404408660a4d976e2ac25881cb1e1f2712f2d430/tools/profiler/core/MicroGeckoProfiler.cpp#47-88
do you think we could add one?

Good idea, added it.

canova · 2022-12-06T12:27:38Z

+        const searchableFields = schema.data.filter((field) =>
+          searchableFieldKeys.includes(field.key)
+        );
+        for (const field of searchableFields) {


Right, changed it, thanks :)

canova · 2022-12-06T12:29:10Z

+            // 'target' wasn't included in our code before. But I thought this
+            // would be a useful addition.


Yeah, it looks like this was added after the initial implementation.

canova · 2022-12-06T12:33:45Z

+          case 'FileIO': {
+            searchableFieldKeys = [
+              'filename',
+              'operation',
+              'source',
+              'threadId',
+            ];


It's probably not. I wasn't sure if they were added initially when we implemented the markers 2.0 or added later but it looks like they are actually added while we are implementing schemas. So we don't need to change them except threadId. We don't have the threadId in the schema, so added it manually.

canova · 2022-12-06T12:38:45Z

+  markerSchema: MarkerSchema,
+  marker: Marker,
+  searchRegExp: RegExp
+): boolean {


Yeah, that makes sense. Added it.

julienw

looks good to me, thanks for checking!

…rsion 33. When the test was initially added, the markerSchema was missing from the profile. That was invalid because version 33 is the version which started requiring that the markerSchema field is present. In firefox-devtools#4352 we added because the upgrader for version 44 caught the fact that it was missing, because it (rightfully) assumes that it's present. However, since this is a profile of a fixed version, we shouldn't attempt to share code with other tests - anything that's shared will usually follow the most recent format version, whereas this test must make sure the profile it creates complies with the hardcoded fixed version.

…rsion 33. When the test was initially added, the markerSchema was missing from the profile. That was invalid because version 33 is the version which started requiring that the markerSchema field is present. In firefox-devtools#4352 we added the missing markerSchema because the upgrader for version 44 caught the fact that it was missing, because it (rightfully) assumes that it's present. However, since this is a profile of a fixed version, we shouldn't attempt to share code with other tests - anything that's shared will usually follow the most recent format version, whereas this test must make sure the profile it creates complies with the hardcoded fixed version.

canova requested a review from julienw December 2, 2022 14:38

julienw approved these changes Dec 5, 2022

View reviewed changes

canova force-pushed the marker-schema-searchable branch from 36961a3 to 35c1c5c Compare December 6, 2022 12:40

canova commented Dec 6, 2022

View reviewed changes

canova requested a review from julienw December 6, 2022 12:41

julienw approved these changes Dec 6, 2022

View reviewed changes

canova added 4 commits December 9, 2022 16:52

Change the marker searching to use the marker schema instead

3e19491

Add an upgrader step to properly support older profiles

8ff2377

Update the tests and snapshots

802f8f2

Address the review comments

8de1e7b

canova force-pushed the marker-schema-searchable branch from 35c1c5c to 8de1e7b Compare December 9, 2022 15:53

canova enabled auto-merge December 9, 2022 15:54

canova merged commit 4a00e43 into firefox-devtools:main Dec 9, 2022

		// 'target' wasn't included in our code before. But I thought this
		// would be a useful addition.

Uh oh!

Conversation

canova commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

canova left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

canova commented Dec 2, 2022 •

edited

Loading

codecov Bot commented Dec 2, 2022 •

edited

Loading

canova left a comment •

edited

Loading